Hi @jshaw35 , I've fixed the CI tests in #88 and #89. Please can you merge the changes into your branch so that this change can be tested? Thanks!

Hi @jshaw35 please can you merge the latest changes (#90) again? These new changes should produce a new set of known good outputs for all compilers. If it all works as expected then we'll be in position to review this change.

Hi @alejandrobodas, I think that I merged in the latest changes. But it looks like the integration failed again. If I am doing something incorrectly just let me know. Happy to continue pulling in from the latest changes.

@jshaw35 I think we should expect some answer changes here.
@takmichibata as the original developer, what are your thoughts?

Hi @jshaw35 . Yes, the tests will fail because your changes modify the results. This makes the review of this change a bit complex, despite the simplicity of the code changes. We will guide you through the process. Basically, this is done in two steps:

1. We need to confirm that the new outputs are good. The table with the summary of changes show that these diagnostics have changed:
   Variable N AvgDiff MinDiff MaxDiff StDev
   ptcloudsatflag0 1326 9.63e-01 -1.0000e+00 1.9000e+01 2.3373e+00
   ptcloudsatflag1 105 2.3810e-01 -1.0000e+00 1.0000e+00 9.7124e-01
   ptcloudsatflag2 157 -2.73e-01 -1.0000e+00 3.0000e+00 9.8667e-01
   ptcloudsatflag3 112 5.3571e-01 -1.0000e+00 1.0000e+00 8.4440e-01
   ptcloudsatflag4 119 2.2637e-01 -1.0000e+00 1.0000e+00 9.6820e-01
   ptcloudsatflag5 146 1.5205e+00 -1.0000e+00 1.9000e+01 3.40e+00
   ptcloudsatflag6 36 1.0000e+00 1.0000e+00 1.0000e+00 0.0000e+00
   ptcloudsatflag7 68 1.0000e+00 1.0000e+00 1.0000e+00 0.0000e+00
   ptcloudsatflag8 24 1.0000e+00 1.0000e+00 1.0000e+00 0.0000e+00
   ptcloudsatflag9 3 1.0000e+00 1.0000e+00 1.0000e+00 0.0000e+00
   cloudsatpia 1028 7.9773e-01 -1.0000e+00 2.5667e+01 2.3142e+00
   npdfcld 283 -4.2403e+27 -3.3333e+29 1.0000e+00 3.6072e+28
   npdfdrz 278 -3.5971e+27 -1.0000e+30 -1.0000e+00 5.9868e+28
   npdfrain 278 -1.0000e+00 -1.0000e+00 -1.0000e+00 0.0000e+00
   ncfodd1 207960 -7.5129e+ -1.0000e+30 1.0000e+00 6.7633e+27
   ncfodd2 207903 -1.1357e+ -1.0000e+30 1.0000e+00 8.9176e+
   ncfodd3 207803 -9.7996e+25 -1.0000e+ -1.0000e+00 8.5951e+27

Please can you add to this PR plots of the diagnostics that have changed? In order to do this, you'll need to download and unpack the outputs generated by one of the compilers, e.g. outputs.gfortran-12.UKMO.tgz. You'll find the files at the bottom of the failed tests:
https://github.com/CFMIP/COSPv2.0/actions/runs/8379994107

The files contain plots of the outputs that have been generated automatically by the integration tests, but unfortunately they don't contain plots of the variables that are affected by this PR, so you'll need to produce the plots offline. It is sufficient to add plots from the outputs of gfortran-12 only.

2. Assuming that the plots look as expected from the code changes, then we'll show you how to modify the tests so that the new outputs are used as reference in the tests. This is easy to do. Once this is done, the tests will pass and we will be able to merge your change.

I hope this helps. Please let us know if you need more information and thanks for contributing to COSP!

Hi @alejandrobodas, I've finally gotten around to producing the plots of the diagnostics that changed. I updated plot_test_outputs.py, so plots of the missing variables should now be produced during CI (here I've assumed some bounds on the CFODD_NDBZE and CFODD_NICOD axes).

The updated plots after the recent failed CI are here:
https://github.com/jshaw35/COSPv2.0/actions/runs/8639121555

I don't actually use the diagnostic fields that were affected by the indexing error, so I will leave interpretation of the updated plots to you. Just reviewing them, however, I noticed that the histograms for the ncfodd1,2,3 variables almost always have in-cloud optical depth values in the 0-2 bin.

Best,
Jonah

@jshaw35 I think we should expect some answer changes here.
@takmichibata as the original developer, what are your thoughts?

Sorry for the late response. I missed the PR notification.
The changes seem fine to me. Thanks Jonah and Dustin.

This PR has been stalled for a while. The code changes look fine, but as @jshaw35 mentions above the plots of the fields affected by the indexing don't look right to me. @takmichibata please can you have a look at the plots attached?

Thank you for sharing the plots @alejandrobodas @jshaw35.
Variables for the # of non-precipitating, drizzling, and precipitating clouds look fine to me. Only the daytime area can be plotted because the warm-rain diagnostics uses MODIS visible retrievals. Samples are between 0 and 20, the maximum number 20 will be consistent with the subcolumns.
On the other hand, I found it strange that sample numbers of ncfodd* are less than 1, but I can understand if the plots are global average values. I also feel no problem that many samples concentrate 0-2 bin of the ICOD because this is not the total (columnar) optical depth but vertically-sliced layered optical depth (i.e., the 0-2 ICOD bin corresponds to near cloud-top). CFODD will be very noisy when we only use a smaller sampling period or snapshot like the offline KGO test.

@alejandrobodas @takmichibata @dustinswales
I'm sorry this fell off of my radar. What are the next steps to merge this in and update the KGOs?

Best,
Jonah

@alejandrobodas @dustinswales @jshaw35
The CFODD outputs, as explained above, shouldn't be considered problematic, so I think it's fine to go ahead and close this PR.

@jshaw35 Can you sync this branch with master?
Thanks

@dustinswales I don't think I have permissions to merge the changes. I don't see any options to merge from my side.

I just merged with the master, is that what you were asking for?

@jshaw35 That's it. Thanks!

@dustinswales is there anything left to do here to pull these changes in and update the kgos?

@jshaw35 The code is good to go, so I think just updating the KGOs and the md5 files (https://github.com/CFMIP/COSPv2.0/blob/master/contributing.md#code-review)

@alejandrobodas How should Jonah provide new KGOs?

Hi @jshaw35 . I've just merged a pull request, please can you sync with master? I'll then upload the new KGOs to Gdrive and share with you their URLs so that you can update the scripts that downloads the KGOs.

@alejandrobodas I think things should be ready to go now!

The gfortran CI tests have failed again. I've re-run the failed tests to produce a second set of outputs with exactly the same version of the code. Below I show the first few values of the variable ptcloudsatflag0 tests on model levels:

ptcloudsatflag0 = 3.265618e-26, 4.557583e-41, 3.265618e-26, 4.557583e-41, 
ptcloudsatflag0 = -2.250033e-16, 4.589533e-41, -2.250033e-16, 4.589533e-41,

As you can see, each run produces different results, which explains why the CI tests fail even after updating the KGOs. Obviously, we can't merge this branch without fixing this. Any ideas for how to investigate the cause of this bug very welcome!

@alejandrobodas I'm taking a look right now.

@alejandrobodas @jshaw35 I was able to trace back the reproducibility issue to quickbeam.F90.

The Cloudsat precipitation occurrence diagnostics are only computed when use_vgrid=.true., but they are always output from quickbeam_column.
This a problem since they are not initialized when use_vgrid=.false., which is true for the "model_levels" UM test, so they get filled with garbage and hence not reproducible.

I added a step to initialize these diagnostics and it seems to have solved the problem.

But... if these diagnostics are to only be used when use_vgrid=.true., we should turn them off in the cosp_output namelist for the UM "model_levels" test, and remove these fields from the KGOs?

For extra safety, I added a guard to the offline driver to not request these diagnostics from COSP when use_vgrid=.false.

@dustinswales thanks for looking into this. I agree that we should turn those diagnostics off in the namelist and delete them from the KGO. The changes in the branch csat_precip_mlev_off do that. I've integrated your changes into that branch. I could initiate a PR with those changes and, once those changes are merged then we could proceed with updating the KGOs from this PR.

@alejandrobodas Sounds good to me.

@jshaw35 please can you sync your branch with the master branch? Hopefully, we're nearly there!

@alejandrobodas I have merged the changes but I may have reverted some of the diagnostics changes accidentally. Let me know how to proceed.

@jshaw35 Do you want to undo the changes of a particular commit? You can do that with git revert <commit_hash>.

@alejandrobodas I think I manually reverted those changes in the last commit 77ee04e
If those changes look right to you hopefully we can proceed with updating the kgos.

@alejandrobodas pinging this again

@jshaw35 sorry, I'm away with access only from my mobile. I'll replace the KGOs next week when I'm back to work.

@jshaw35 I've uploaded the new gfortran KGOs to GDrive. Please can you move to v006 by picking up the changes in the branch KGOs_5_6? Thanks!

@alejandrobodas sorry for the long delay, I was also away from work last week. I merged with the KGOs_5_6 branch but there still seem to be some differences KGOs_5_6...jshaw35:COSPv2.0:cloudsat_bugfix
Let me know how to proceed. Also tagging @dustinswales since he can likely work through things much faster than me right now.

@jshaw35 the tests are failing because there is a mismatch in the KGO versions for the gfortran compilers. Please use v006 in .github/workflows/continuous_integration.yml#L44. You'll also need to make sure that the md5 sums for v005 and v006 of the gfortran KGOs match the ones in my branch. These are defined in the .md5 files stored in the directory driver/data/outputs/UKMO. These are the (hopefully) correct values:

98893d9574e75430eee342feeeb5f204  cosp2_output.um_global.gfortran.kgo.v005.nc
98893d9574e75430eee342feeeb5f204  cosp2_output.um_global.gfortran.kgo.v006.nc
e956d433d07f6b0e77c5f0d0c4656137  cosp2_output.um_global_model_levels.gfortran.kgo.v005.nc
6926dc2c81b7f89978e69902f8604672  cosp2_output.um_global_model_levels.gfortran.kgo.v006.nc

Success!! The integration tests have now passed. This looks good to me. @dustinswales are you happy for me to go ahead and merge?

Changes merged. This has been a small but challenging change.
@jshaw35 thanks for your contribution and for your patience!

@alejandrobodas and @dustinswales thank you for your support and patience! One last request, can you create a new tag for this version of the model? CESM would like to incorporate this version of COSP and update the CAM driver before incorporating COSP-RTTOV and their procedures require a tagged versio.
Also, I will open a PR for COSP-RTTOV within the next week or so!

@jshaw35 Here you go, https://github.com/CFMIP/COSPv2.0/releases/tag/v2.1.8.
@alejandrobodas FWIW It's been five years since our last tag, which I incremented by one for the tag.